core(lantern): allow non-XHRs to depend on CPU Nodes #11767

warrengm · 2020-12-04T15:59:43Z

Summary
This fixes an issue that @jburger424 found in a test page where we deliberately inserted a long task before loading a script, but that wasn't reflected in results. (So the simulated load time was actually better than the real load time)

More generally I don't think we want to special case XHRs since it ignores fetch and jsonp requests which are similar enough.
Impact is as follows:

I found that excluding images and stylesheets does reduce LCP error--which is somewhat surprising because I would expect those edges to be more accurate for pages that dynamically insert images. I'd think we want to capture the impact of long tasks on LCP in those cases.

Related Issues/PRs
It seems to have been implemented this way from the start: #3162. Not sure why the decision was made to ignore XHRs though.

patrickhulce · 2020-12-04T16:41:29Z

Thanks so much @warrengm this looks like a great improvement from the numbers! 🎉

Conceptually they were excluded because ResourceSendRequest happened for all network initiation activity that happens to occur in a task, not just activity directly caused by the currently executing script. That was too wide of a net to cast back in 2017 based on scheduling, prevalence of XHR capturing many important cases already, our poor ability to detect graph cycles, etc, so we generally stayed conservative unless there was a definitive win in the error numbers. I definitely believe enough changes have moved in our favor over the past 3 years for the calculus to have flipped here though, and we just weren't actively revisiting any of it so thank you :)

My hunch on what you're seeing with images and stylesheets is what remains of that original concern, but definitely appears to be a big win overall 🎉

More generally I don't think we want to special case XHRs since it ignores fetch and jsonp requests which are similar enough.

For sure, the safest version of this change that sticks with the original conservativeness would be only expanding this to fetch and other scripts rather than all network requests. If that preserves a majority of the accuracy improvements, that'd probably be my ideal change, but if the aggregate wins do come from other types, then I think we're in a much better place to proceed there today too than we were in 2017.

warrengm · 2020-12-04T18:58:13Z

Updating the condition to improve accuracy sounds good!

Carving out Image and Stylesheets had an improvement, but carving out Other and undefined was worse in my testing. However I hadn't tried just xhr, fetch, and jsonp yet. I'll run a few more tests and let you know what I find.

I also had some thoughts about it probably matters whether the request was initiated at the start of the task or the end, but that is probably too hard to model now.

connorjclark · 2020-12-07T20:55:08Z

FYI our target release for v7 is Wed/Th. Let us know if you need an assist here.

warrengm · 2020-12-09T15:19:40Z

I'll have the PR ready by later today--I mostly need to finish updating tests.

After some testing, I think excluding images, stylesheets, and fonts is a good option to reduce the error on LCP results (with some trade-offs in a smaller improvement on speed index simulation):

patrickhulce · 2020-12-09T17:08:45Z

sounds good @warrengm let me know if you need any help here!

warrengm · 2020-12-09T19:23:02Z

I think this is good to review. The failures seem to be the same that have been discussed for other PRs

patrickhulce

cool cool cool! the redirectedResourceType and one test diff only obstacles I see, looks great otherwise, thanks for this! 💯

lighthouse-core/lib/network-request.js

patrickhulce · 2020-12-09T19:18:26Z

lighthouse-core/test/audits/byte-efficiency/__snapshots__/byte-efficiency-audit-test.js.snap

-  "default": 1330,
-  "justTTI": 800,
+  "default": 960,
+  "justTTI": 960,


we might need to adjust the test graph we use so these are different

patrickhulce · 2020-12-09T19:19:44Z

lighthouse-core/test/audits/byte-efficiency/byte-efficiency-audit-test.js

@@ -266,7 +266,7 @@ describe('Byte efficiency base audit', () => {
    const result = await MockAudit.audit(artifacts, {settings, computedCache});
    const resultTti = await MockJustTTIAudit.audit(artifacts, {settings, computedCache});
    // expect less savings with just TTI
-    expect(resultTti.numericValue).toBeLessThan(result.numericValue);
+    expect(resultTti.numericValue).toBeLessThanOrEqual(result.numericValue);


this assertion was hard coded because it is the test :) we'll need to construct a different graph to get the same value if TTI is the same as the last node timing now

Is it cool to have computeWasteWithTTIGraph just return a fixed number? IMO that would make it clear that were testing subclassing.

That's fine if we don't have any other traces that are different in this regard. I'd be bummed to lose a bit of the double duty testing that computeWasteWithTTIGraph does its job correctly, but we can address that another time.

That makes sense. I cheated to get the test to pass now since I'm not sure how to construct the right graph.

Here it might be good to have each unit tests test one thing. So maybe test computeWasteWithTTIGraph directly by calling it with different options or at least have sibling classes where you're not testing overriding.

:) you're right of course and we should finally get around to that since this base is the foundation of many different opportunities. filed #11802 👍

lighthouse-core/test/fixtures/lantern-master-accuracy.json

lighthouse-core/test/fixtures/lantern-master-computed-values.json

lighthouse-core/computed/page-dependency-graph.js

patrickhulce · 2020-12-09T19:26:24Z

shoot, it looks like basics weren't failing because test-lantern wasn't finding the touched files anymore 🤦

lighthouse/lighthouse-core/scripts/test-lantern.sh

Line 12 in 111aaf1

CHANGED_FILES=$(git --no-pager diff --name-only $TRAVIS_COMMIT_RANGE)

patrickhulce

impl looks great, thanks for sticking with this @warrengm! would you be able to rebase with master to incorporate #11801 just to make sure the lantern tests are good in CI too before we merge?

lighthouse-core/lib/network-request.js

…nto simulation

warrengm · 2020-12-10T20:25:54Z

Done. Rerunning the checks now!

paulirish · 2020-12-10T21:36:39Z

Done. Rerunning the checks now!

booyah!

Allow non-XHRs to depend on CPU Nodes

6861fae

google-cla bot added the cla: yes label Dec 4, 2020

vercel bot deployed to Preview December 4, 2020 15:59 View deployment

patrickhulce added 7.0 breaking labels Dec 4, 2020

warrengm added 2 commits December 4, 2020 16:26

Only link CPU nodes with fetch, XHR, and scripts

f475920

Update snapshot tests

faf4d7b

patrickhulce self-assigned this Dec 4, 2020

patrickhulce added the waiting4committer label Dec 4, 2020

warrengm added 3 commits December 9, 2020 10:37

Fix unit tests

d38eb6e

Run yarn update:lantern-master

486a4b4

Formatting

91c2b92

vercel bot deployed to Preview December 9, 2020 15:39 View deployment

Merge with main

5108678

vercel bot deployed to Preview December 9, 2020 16:17 View deployment

Run yarn update:sample-json

776a7e4

warrengm changed the title ~~core(lantern): Allow non-XHRs to depend on CPU Nodes~~ core(lantern): allow non-XHRs to depend on CPU Nodes Dec 9, 2020

vercel bot deployed to Preview December 9, 2020 18:59 View deployment

warrengm marked this pull request as ready for review December 9, 2020 19:00

warrengm requested a review from a team as a code owner December 9, 2020 19:00

warrengm requested review from adamraine and removed request for a team December 9, 2020 19:00

Fix type checking:

7efff16

vercel bot deployed to Preview December 9, 2020 19:12 View deployment

patrickhulce reviewed Dec 9, 2020

View reviewed changes

warrengm requested a review from patrickhulce December 9, 2020 19:26

Fix tests and types

6b38134

vercel bot deployed to Preview December 9, 2020 20:09 View deployment

connorjclark mentioned this pull request Dec 9, 2020

Move remaining travis tests to github actions #11194

Closed

4 tasks

Update byte efficiency test

be5f56c

vercel bot deployed to Preview December 9, 2020 21:21 View deployment

This was referenced Dec 9, 2020

tests: reenable change-related tests in github actions #11801

Merged

Add more test cases to the byte efficiency savings estimations #11802

Closed

patrickhulce approved these changes Dec 10, 2020

View reviewed changes

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

warrengm added 2 commits December 10, 2020 15:24

Revert unrelated change

1028185

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

2ff4a66

…nto simulation

vercel bot deployed to Preview December 10, 2020 20:24 View deployment

patrickhulce merged commit 7915708 into GoogleChrome:master Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lantern): allow non-XHRs to depend on CPU Nodes #11767

core(lantern): allow non-XHRs to depend on CPU Nodes #11767

warrengm commented Dec 4, 2020

patrickhulce commented Dec 4, 2020

warrengm commented Dec 4, 2020

connorjclark commented Dec 7, 2020

warrengm commented Dec 9, 2020

patrickhulce commented Dec 9, 2020

warrengm commented Dec 9, 2020

patrickhulce left a comment

patrickhulce Dec 9, 2020

patrickhulce Dec 9, 2020

warrengm Dec 9, 2020

patrickhulce Dec 9, 2020

warrengm Dec 9, 2020

patrickhulce Dec 9, 2020

patrickhulce commented Dec 9, 2020 •

edited

Loading

patrickhulce left a comment

warrengm commented Dec 10, 2020

paulirish commented Dec 10, 2020

core(lantern): allow non-XHRs to depend on CPU Nodes #11767

core(lantern): allow non-XHRs to depend on CPU Nodes #11767

Conversation

warrengm commented Dec 4, 2020

patrickhulce commented Dec 4, 2020

warrengm commented Dec 4, 2020

connorjclark commented Dec 7, 2020

warrengm commented Dec 9, 2020

patrickhulce commented Dec 9, 2020

warrengm commented Dec 9, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Dec 9, 2020

Choose a reason for hiding this comment

patrickhulce Dec 9, 2020

Choose a reason for hiding this comment

warrengm Dec 9, 2020

Choose a reason for hiding this comment

patrickhulce Dec 9, 2020

Choose a reason for hiding this comment

warrengm Dec 9, 2020

Choose a reason for hiding this comment

patrickhulce Dec 9, 2020

Choose a reason for hiding this comment

patrickhulce commented Dec 9, 2020 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

warrengm commented Dec 10, 2020

paulirish commented Dec 10, 2020

patrickhulce commented Dec 9, 2020 •

edited

Loading